Skip to content

http: create header field names as internalized strings#64277

Open
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:http-internalize-header-names
Open

http: create header field names as internalized strings#64277
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:http-internalize-header-names

Conversation

@araujogui

@araujogui araujogui commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Internalize header field names so repeated occurrences resolve to the same string in the isolate's string table, speeding up matchKnownFields() comparisons and req.headers builds.

Benchmark 
http/incoming_headers.js duration=5 w=0 headers=20 connections=50 benchmarker='wrk'         **      3.20 %       ±2.00% ±2.66% ±3.47%
http/incoming_headers.js duration=5 w=6 headers=20 connections=50 benchmarker='wrk'        ***      3.75 %       ±1.99% ±2.65% ±3.45%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@araujogui
http: create header field names as internalized strings
f1a205d 
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Copilot AI review requested due to automatic review settings July 4, 2026 03:16
@nodejs-github-bot

nodejs-github-bot commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Jul 4, 2026
Copilot AI reviewed Jul 4, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes HTTP header processing by internalizing header field name strings so repeated header names can compare faster (e.g., in JS matchKnownFields()) and reduce duplicate string allocations when building req.headers.

Changes:

  • Add StringPtr::ToInternalizedString() to create V8 internalized strings from header field names.
  • Use internalized strings for header field names when constructing the headers array passed to JS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/node_http_parser.cc
@araujogui
http: only internalize short header field names
fff2fbc 
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2026
@nodejs-github-bot

nodejs-github-bot commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74629/

@nodejs-github-bot

nodejs-github-bot commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74631/

jasnell
jasnell reviewed Jul 8, 2026
View reviewed changes
Comment thread src/node_http_parser.cc
return String::NewFromOneByte(env->isolate(),
reinterpret_cast<const uint8_t*>(str_),
NewStringType::kInternalized,
size_)

@jasnell jasnell Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. I'm not convinced this is a good idea.

While it certainly helps legitimate cases, there's a risk here.

A malicious sender could leverage this into a memory leak or cause significant additional GC pressure pretty easily by sending a large number of intentionally different header values at a high rate. Internalized strings do get cleaned up during a major GC. The current existing non-internalized strings can be cleaned up in a minor GC.

We really ought to be internalizing only known header names.

@araujogui araujogui Jul 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, should I apply the same fix on the HTTP/2 path?

node/src/node_http_common-inl.h

Line 126 in 8e1ab55

name_.reset(name, true); // Internalizable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
Copilot code review Copilot Copilot left review comments
@pimterry pimterry pimterry approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@araujogui @nodejs-github-bot @jasnell @pimterry